[PassBuilder] Add RelLookupTableConverterPass to LTO #124053

gulfemsavrun · 2025-01-23T03:07:38Z

[PassBuilder] Add RelLookupTableConverterPass to LTO

This patch adds RelLookupTableConverterPass into the LTO
post-link optimization pass pipeline. This optimization
converts lookup tables to relative lookup tables to make
them PIC-friendly, which is already included in the non-LTO
pass pipeline. This patch adds this optimization to the
post-link optimization pipeline to discover more
opportunities in the LTO context.

ilovepi · 2025-01-24T22:34:06Z

llvm/lib/Passes/PassBuilderPipelines.cpp

  // Remove unused arguments from functions.
  MPM.addPass(DeadArgumentEliminationPass());

+  MPM.addPass(RelLookupTableConverterPass());


Shouldn't this be much later in the pipeline? I'd guess after MergeFunctions, but before CGProfile is probably a better spot(

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Line 2123 in 77c23fd

) for high opt levels. You'll probably want to add it before the returns for lower opt levels too.

I suppose it could arguably be added to Late EP callbacks, as that would also apply to other optimization levels, w/o needing to specifically add the pass everywhere.

CC: @aeubanks @nikic @efriedma-quic

I moved the pass later in the LTO pipeline. The early return for other optimization levels are for -O0 and -O1, and I think it is reasonable to add this pass when built with -O2 and higher.

Well, I was thinking that wanting PIC friendly lookup tables isn't necessarily something tied to the optimization level. For instance, I'd suppose if you care about PIC friendliness, you'd want it on even at low opt levels. That's also something easy to revisit, though, so its probably fine to start here and revisit as needed.

Let's revisit this because I'm not sure whether this optimization should be included in O0 or O1 because those opt levels are limited in terms of the passes that they include. This is not a costly optimization, but let's follow up on this.

ilovepi

Do we need to update any tests? I seem to recall several pipeline tests that checked the order of passes. I'm not sure if we have any of those for LTO, though, as I don't see them in the test dir.

Assuming CI is happy w/ this, LGTM, but lets confirm w/ @aeubanks or @nikic before landing. Pass ordering has a tendency to have unintended consequences, so I'd feel better if one of them double checked me on that, since we're so close to the branch point.

ilovepi · 2025-01-27T19:40:45Z

Oh, does moving a pass in the pipeline need a release note?

nikic

I do believe you need to adjust pipeline tests, but the change itself LGTM.

I'd also drop this TODO:

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Lines 1597 to 1599 in f569585

    
           // TODO: Relative look table converter pass caused an issue when full lto is 
        
           // enabled. See https://reviews.llvm.org/D94355 for more details. 
        
           // Until the issue fixed, disable this pass during pre-linking phase.

While running the pass in that position ran into an issue, the pass should not be run pre-link in any case, even if that issue is resolved.

aeubanks

please add some background/motivation in the commit description, and explicitly mention "RelLookupTableConverterPass" in the commit title

gulfemsavrun · 2025-01-28T01:14:35Z

I do believe you need to adjust pipeline tests, but the change itself LGTM.

I'd also drop this TODO:

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Lines 1597 to 1599 in f569585

// TODO: Relative look table converter pass caused an issue when full lto is

// enabled. See https://reviews.llvm.org/D94355 for more details.

// Until the issue fixed, disable this pass during pre-linking phase.

While running the pass in that position ran into an issue, the pass should not be run pre-link in any case, even if that issue is resolved.

I removed that todo, and adjusted pipeline tests.

gulfemsavrun · 2025-01-28T01:14:53Z

please add some background/motivation in the commit description, and explicitly mention "RelLookupTableConverterPass" in the commit title

Done.

gulfemsavrun · 2025-01-28T01:17:50Z

Oh, does moving a pass in the pipeline need a release note?

Which release note should this go in? Clang maybe?
https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst

aeubanks · 2025-01-28T22:03:16Z

llvm/docs/ReleaseNotes.md is the place to add release notes for pipeline changes

I think for the final commit to have the commit message/description, the PR's title and first comment must contain it, at least for squash and merge like LLVM does

ilovepi · 2025-01-28T22:54:28Z

llvm/docs/ReleaseNotes.md is the place to add release notes for pipeline changes

I think for the final commit to have the commit message/description, the PR's title and first comment must contain it, at least for squash and merge like LLVM does

Agreed that's a best practice, but I'm pretty sure if you're merging through github UI you can also edit the commit message and title before landing.

This patch adds RelLookupTableConverterPass into the LTO post-link optimization pass pipeline. This optimization converts lookup tables to relative lookup tables to make them PIC-friendly, which is already included in the non-LTO pass pipeline. This patch adds this optimization to the post-link optimization pipeline to discover more opportunities in the LTO context.

gulfemsavrun · 2025-01-28T23:07:26Z

llvm/docs/ReleaseNotes.md is the place to add release notes for pipeline changes
I think for the final commit to have the commit message/description, the PR's title and first comment must contain it, at least for squash and merge like LLVM does

Agreed that's a best practice, but I'm pretty sure if you're merging through github UI you can also edit the commit message and title before landing.

Oh, I did not realize that commit message is not updated after I updated in the branch itself. Fixed it now!

gulfemsavrun mentioned this pull request Jan 23, 2025

[ConstantMerge] Only merge constant w/unnamed_addr #124050

Open

gulfemsavrun requested a review from ilovepi January 23, 2025 23:24

ilovepi reviewed Jan 24, 2025

View reviewed changes

gulfemsavrun force-pushed the lto-add-rel-lookup-table-pass-post-link branch from de922a3 to f569585 Compare January 27, 2025 19:11

ilovepi approved these changes Jan 27, 2025

View reviewed changes

nikic approved these changes Jan 27, 2025

View reviewed changes

aeubanks reviewed Jan 27, 2025

View reviewed changes

gulfemsavrun force-pushed the lto-add-rel-lookup-table-pass-post-link branch from f569585 to dca6468 Compare January 28, 2025 01:12

gulfemsavrun force-pushed the lto-add-rel-lookup-table-pass-post-link branch from dca6468 to 73aa225 Compare January 28, 2025 23:05

gulfemsavrun changed the title ~~[PassBuilder] Add a pass to LTO postlink step~~ [PassBuilder] Add RelLookupTableConverterPass to LTO Jan 28, 2025

gulfemsavrun merged commit 3890215 into llvm:main Jan 28, 2025
6 of 8 checks passed

	// TODO: Relative look table converter pass caused an issue when full lto is
	// enabled. See https://reviews.llvm.org/D94355 for more details.
	// Until the issue fixed, disable this pass during pre-linking phase.

[PassBuilder] Add RelLookupTableConverterPass to LTO #124053

[PassBuilder] Add RelLookupTableConverterPass to LTO #124053

Uh oh!

Conversation

gulfemsavrun commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilovepi Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

gulfemsavrun Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

ilovepi Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

gulfemsavrun Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

ilovepi commented Jan 27, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

gulfemsavrun commented Jan 28, 2025

Uh oh!

gulfemsavrun commented Jan 28, 2025

Uh oh!

gulfemsavrun commented Jan 28, 2025

Uh oh!

aeubanks commented Jan 28, 2025

Uh oh!

ilovepi commented Jan 28, 2025

Uh oh!

gulfemsavrun commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gulfemsavrun commented Jan 23, 2025 •

edited

Loading

gulfemsavrun commented Jan 28, 2025 •

edited

Loading